fix: Add validation to ensure added auth token getters are used by the tool #220
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anubhav756
force-pushed
the
anubhav-tool-unused-auth
branch
2 times, most recently
from
06847ff to
4405195
Compare
anubhav756
changed the base branch from
anubhav-error
to
anubhav-fix-authz-required
anubhav756
force-pushed
the
anubhav-fix-authz-required
branch
from
854ef47 to
e664e96
Compare
anubhav756
force-pushed
the
anubhav-tool-unused-auth
branch
from
4405195 to
3232752
Compare
kurtisvg
approved these changes
May 12, 2025
anubhav756
force-pushed
the
anubhav-fix-authz-required
branch
from
e664e96 to
6740434
Compare
…e tool Previously, the `ToolboxTool.add_auth_token_getters` method only validated against existing registered getters or conflicts with client headers. It did not verify if *all* the auth token getters provided were actually used or required by the specific tool instance they were being added to. This PR enhances the validation in `add_auth_token_getters`. It now leverages the `used_auth_token_getters` information returned by the existing `identify_required_authn_params` call. This allows the method to confirm that every getter passed in is genuinely required by the tool, raising an error if any are unused. This ensures that only relevant auth token getters are attempted to be registered for a tool, preventing misconfigurations and human errors. > [!NOTE] > This validation aligns with the existing validation logic already present in the `ToolboxClient.load_tool` method, promoting a consistent approach to handling auth token getter requirements across the codebase.
anubhav756
force-pushed
the
anubhav-tool-unused-auth
branch
from
3232752 to
88175d7
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Previously, the
ToolboxTool.add_auth_token_gettersmethod only validated against existing registered getters or conflicts with client headers. It did not verify if all the auth token getters provided were actually used or required by the specific tool instance they were being added to.Solution
This PR enhances the validation in
add_auth_token_getters. It now leverages theused_auth_token_gettersinformation returned by the existingidentify_required_authn_paramscall. This allows the method to confirm that every getter passed in is genuinely required by the tool, raising an error if any are unused.Benefit
This ensures that only relevant auth token getters are attempted to be registered for a tool, preventing misconfigurations and human errors.
Note
This validation aligns with the existing validation logic already present in the
ToolboxClient.load_toolmethod, promoting a consistent approach to handling auth token getter requirements across the codebase.